Introduce `ExhaustiveRefasterTypeMigration` check #770

Stephan202 · 2023-08-29T06:24:30Z

Suggested commit message:

Introduce `ExhaustiveRefasterTypeMigration` check (#770)

The new `@TypeMigration` annotation can be placed on Refaster rule
collections to indicate that they migrate most or all public methods of
an indicated type. The new check validates the claim made by the
annotation.

This will help flag new to-be-migrated methods any time the JUnit or TestNG dependency is upgraded. (This PR also flags that TestNGToAssertJRules is no longer nearly as exhaustive as it once was. Resolving that will be deferred to a separate PR. I started a TestNG and JUnit branch. That said, given the repetition involved, perhaps introducing a bug checker or two is in order.)

github-actions · 2023-08-29T06:26:52Z

Surviving mutants in this change: 7
Killed mutants in this change: 33

class	surviving	killed
🧟`tech.picnic.errorprone.bugpatterns.ExhaustiveRefasterTypeMigration`	5	26
🧟`tech.picnic.errorprone.bugpatterns.ExhaustiveRefasterTypeMigration$1`	2	6
🎉`tech.picnic.errorprone.bugpatterns.StringJoin`	0	1

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

github-actions · 2023-08-29T07:01:21Z

Surviving mutants in this change: 7
Killed mutants in this change: 33

class	surviving	killed
🧟`tech.picnic.errorprone.bugpatterns.ExhaustiveRefasterTypeMigration`	5	26
🧟`tech.picnic.errorprone.bugpatterns.ExhaustiveRefasterTypeMigration$1`	2	6
🎉`tech.picnic.errorprone.bugpatterns.StringJoin`	0	1

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

sonarqubecloud · 2023-08-29T07:27:42Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
3 Code Smells

98.7% Coverage
0.0% Duplication

github-actions · 2023-08-29T07:28:21Z

Surviving mutants in this change: 7
Killed mutants in this change: 31

class	surviving	killed
🧟`tech.picnic.errorprone.bugpatterns.ExhaustiveRefasterTypeMigration`	5	26
🧟`tech.picnic.errorprone.bugpatterns.ExhaustiveRefasterTypeMigration$1`	2	4
🎉`tech.picnic.errorprone.bugpatterns.StringJoin`	0	1

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

github-actions · 2023-10-08T13:17:20Z

Surviving mutants in this change: 7
Killed mutants in this change: 31

class	surviving	killed
🧟`tech.picnic.errorprone.bugpatterns.ExhaustiveRefasterTypeMigration`	5	26
🧟`tech.picnic.errorprone.bugpatterns.ExhaustiveRefasterTypeMigration$1`	2	4
🎉`tech.picnic.errorprone.bugpatterns.StringJoin`	0	1

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

sonarqubecloud · 2023-10-08T13:17:39Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
3 Code Smells

98.7% Coverage
0.0% Duplication

sonarqubecloud · 2023-12-18T16:55:21Z

Quality Gate passed

The SonarCloud Quality Gate passed, but some issues were introduced.

3 New issues
0 Security Hotspots
98.7% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

github-actions · 2023-12-18T16:55:22Z

Surviving mutants in this change: 7
Killed mutants in this change: 30

class	surviving	killed
🧟`tech.picnic.errorprone.bugpatterns.ExhaustiveRefasterTypeMigration`	5	26
🧟`tech.picnic.errorprone.bugpatterns.ExhaustiveRefasterTypeMigration$1`	2	4

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

github-actions · 2023-12-18T16:55:41Z

Surviving mutants in this change: 7
Killed mutants in this change: 30

class	surviving	killed
🧟`tech.picnic.errorprone.bugpatterns.ExhaustiveRefasterTypeMigration`	5	26
🧟`tech.picnic.errorprone.bugpatterns.ExhaustiveRefasterTypeMigration$1`	2	4

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

mohamedsamehsalah

Nothing stands out to me 🚀

...ntrib/src/main/java/tech/picnic/errorprone/bugpatterns/LexicographicalAnnotationListing.java

mohamedsamehsalah · 2024-01-27T19:14:31Z

refaster-support/src/main/java/tech/picnic/errorprone/refaster/annotation/TypeMigration.java

+   *     com.google.errorprone.util.Signatures#prettyMethodSignature(com.sun.tools.javac.code.Symbol.ClassSymbol,
+   *     com.sun.tools.javac.code.Symbol.MethodSymbol)}.
+   */
+  String[] unmigratedMethods() default {};


Intellij flags this as a spelling mistake 🤔
Maybe:

Suggested change

String[] unmigratedMethods() default {};

String[] nonMigratedMethods() default {};

or

Suggested change

String[] unmigratedMethods() default {};

String[] notMigratedMethods() default {};

unmigrated is an actual word, so we can also tell IntelliJ to ignore it :). Would be my preference.

mohamedsamehsalah · 2024-01-27T19:25:51Z

...ib/src/test/java/tech/picnic/errorprone/bugpatterns/ExhaustiveRefasterTypeMigrationTest.java

+            "      of = Util.class,",
+            "      unmigratedMethods = {",
+            "        \"publicStaticVoidMethod()\",",
+            "        \"publicStringMethodWithArg(int)\",",


Do we need to test overloaded methods here ?

Suggested change

" \"publicStringMethodWithArg(int)\",",

" \"publicStringMethodWithArg(int)\",",

" \"publicStringMethodWithArg(String)\",",

Which brings me to my question on why do we need to flag all the variants of the overloaded method ? 🤔

Is it wrong to assume that if a rule does not apply to one type set of arguments, will not be applied to other ?

Nice idea to test overloads; let me add a commit for that.

As for why we should flag all variants: due to how Refaster works, each variant needs to be matched explicitly. It may be that n out of m > n overloads are covered; by explicitly listing the remainder it'll be easier to determine which rules are missing.

Stephan202

Rebased and added two small commits.

...ntrib/src/main/java/tech/picnic/errorprone/bugpatterns/LexicographicalAnnotationListing.java

Stephan202 · 2024-01-28T12:11:23Z

...ib/src/test/java/tech/picnic/errorprone/bugpatterns/ExhaustiveRefasterTypeMigrationTest.java

+            "      of = Util.class,",
+            "      unmigratedMethods = {",
+            "        \"publicStaticVoidMethod()\",",
+            "        \"publicStringMethodWithArg(int)\",",


Nice idea to test overloads; let me add a commit for that.

As for why we should flag all variants: due to how Refaster works, each variant needs to be matched explicitly. It may be that n out of m > n overloads are covered; by explicitly listing the remainder it'll be easier to determine which rules are missing.

github-actions · 2024-01-28T17:24:23Z

Surviving mutants in this change: 7
Killed mutants in this change: 30

class	surviving	killed
🧟`tech.picnic.errorprone.bugpatterns.ExhaustiveRefasterTypeMigration`	5	26
🧟`tech.picnic.errorprone.bugpatterns.ExhaustiveRefasterTypeMigration$1`	2	4

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

Stephan202 · 2024-02-10T13:13:23Z

I'll rebase this branch and move the check to the new guidelines module.

github-actions · 2024-02-10T13:32:00Z

Surviving mutants in this change: 7
Killed mutants in this change: 30

class	surviving	killed
🧟`tech.picnic.errorprone.guidelines.bugpatterns.ExhaustiveRefasterTypeMigration`	5	26
🧟`tech.picnic.errorprone.guidelines.bugpatterns.ExhaustiveRefasterTypeMigration$1`	2	4

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

github-actions · 2024-02-11T09:15:08Z

Surviving mutants in this change: 7
Killed mutants in this change: 30

class	surviving	killed
🧟`tech.picnic.errorprone.guidelines.bugpatterns.ExhaustiveRefasterTypeMigration`	5	26
🧟`tech.picnic.errorprone.guidelines.bugpatterns.ExhaustiveRefasterTypeMigration$1`	2	4

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

rickie

Amazing check 🚀 !

rickie · 2024-01-29T06:53:31Z

refaster-support/src/main/java/tech/picnic/errorprone/refaster/annotation/TypeMigration.java

+ * indicated type.
+ */
+// XXX: Add support for `#unmigratedFields()`.
+// XXX: Consider making this annotation `@Repeatable`, for cases where a single Refaster rules


Suggested change

// XXX: Consider making this annotation `@Repeatable`, for cases where a single Refaster rules

// XXX: Consider making this annotation `@Repeatable`, for cases where a single Refaster rule

rickie · 2024-01-29T06:55:01Z

refaster-support/src/main/java/tech/picnic/errorprone/refaster/annotation/TypeMigration.java

+   *     com.google.errorprone.util.Signatures#prettyMethodSignature(com.sun.tools.javac.code.Symbol.ClassSymbol,
+   *     com.sun.tools.javac.code.Symbol.MethodSymbol)}.
+   */
+  String[] unmigratedMethods() default {};


unmigrated is an actual word, so we can also tell IntelliJ to ignore it :). Would be my preference.

rickie · 2024-02-10T18:13:32Z

refaster-support/src/main/java/tech/picnic/errorprone/refaster/annotation/TypeMigration.java

+// XXX: Add support for `#unmigratedFields()`.
+// XXX: Consider making this annotation `@Repeatable`, for cases where a single Refaster rules
+// collection migrates away from multiple types.
+@Target(ElementType.TYPE)


Not really sure what the best sorting would be here, but we have the annotation sorting on method level. We could swap them here as well.

rickie · 2024-02-10T18:13:43Z

refaster-support/src/main/java/tech/picnic/errorprone/refaster/annotation/TypeMigration.java

+// XXX: Consider making this annotation `@Repeatable`, for cases where a single Refaster rules
+// collection migrates away from multiple types.
+@Target(ElementType.TYPE)
+@Retention(RetentionPolicy.SOURCE)


Suggested change

@Retention(RetentionPolicy.SOURCE)

@Retention(SOURCE)

I think we don''t really lose any context with this? Do we want to add this to the StaticImport check?

I agree that we don't lose much, but the current variant does seem a bit clearer to me. Since this annotation will only be present on a separate line in (almost always) small files, I don't think there's much point in forcing a static import here. Internally only 2 out of 67 usages statically import it.

rickie · 2024-02-10T18:18:03Z

.../java/tech/picnic/errorprone/guidelines/bugpatterns/ExhaustiveRefasterTypeMigrationTest.java

+            "    }",
+            "  }",
+            "",
+            "  // BUG: Diagnostic contains: The set of unmigrated methods listed by the `@TypeMigration`",


Why don't we simply use "// BUG: Diagnostic contains:", here? That already signals that we are just flagging this, right?

This check emits two kinds of messages, so for each violation I'm asserting that the correct message is emitted.

github-actions · 2024-02-11T10:37:33Z

Surviving mutants in this change: 7
Killed mutants in this change: 30

class	surviving	killed
🧟`tech.picnic.errorprone.guidelines.bugpatterns.ExhaustiveRefasterTypeMigration`	5	26
🧟`tech.picnic.errorprone.guidelines.bugpatterns.ExhaustiveRefasterTypeMigration$1`	2	4

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

The new `@TypeMigration` annotation can be placed on Refaster rule collections to indicate that they migrate most or all public methods of an indicated type. The new check validates the claim made by the annotation.

Stephan202

Rebased and added a small commit. Will merge once built.

Stephan202 · 2024-02-11T11:21:39Z

refaster-support/src/main/java/tech/picnic/errorprone/refaster/annotation/TypeMigration.java

+@Retention(RetentionPolicy.SOURCE)
+@Target(ElementType.TYPE)
+public @interface TypeMigration {


On the topic of annotation sorting: makes sense. We should extend LexicographicalAnnotationListing; will add a comment.

Stephan202 · 2024-02-11T11:26:22Z

refaster-support/src/main/java/tech/picnic/errorprone/refaster/annotation/TypeMigration.java

+// XXX: Consider making this annotation `@Repeatable`, for cases where a single Refaster rules
+// collection migrates away from multiple types.
+@Target(ElementType.TYPE)
+@Retention(RetentionPolicy.SOURCE)


I agree that we don't lose much, but the current variant does seem a bit clearer to me. Since this annotation will only be present on a separate line in (almost always) small files, I don't think there's much point in forcing a static import here. Internally only 2 out of 67 usages statically import it.

Stephan202 · 2024-02-11T11:27:42Z

.../java/tech/picnic/errorprone/guidelines/bugpatterns/ExhaustiveRefasterTypeMigrationTest.java

+            "    }",
+            "  }",
+            "",
+            "  // BUG: Diagnostic contains: The set of unmigrated methods listed by the `@TypeMigration`",


This check emits two kinds of messages, so for each violation I'm asserting that the correct message is emitted.

github-actions · 2024-02-11T11:32:19Z

Surviving mutants in this change: 7
Killed mutants in this change: 30

class	surviving	killed
🧟`tech.picnic.errorprone.guidelines.bugpatterns.ExhaustiveRefasterTypeMigration`	5	26
🧟`tech.picnic.errorprone.guidelines.bugpatterns.ExhaustiveRefasterTypeMigration$1`	2	4

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

sonarqubecloud · 2024-02-11T11:32:33Z

Quality Gate passed

Issues
3 New issues

Measures
0 Security Hotspots
98.7% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

Stephan202 added this to the 0.14.0 milestone Aug 29, 2023

Stephan202 requested review from oxkitsune and rickie August 29, 2023 06:24

rickie modified the milestones: 0.14.0, 0.15.0 Oct 4, 2023

Stephan202 force-pushed the sschroevers/introduce-refaster-type-migration-annotation branch from 0da94ff to c085f2d Compare October 8, 2023 13:14

rickie added the new feature label Dec 1, 2023

Stephan202 force-pushed the sschroevers/introduce-refaster-type-migration-annotation branch from c085f2d to 3cfa724 Compare December 18, 2023 16:53

rickie requested a review from mohamedsamehsalah January 22, 2024 06:12

mohamedsamehsalah approved these changes Jan 27, 2024

View reviewed changes

Stephan202 force-pushed the sschroevers/introduce-refaster-type-migration-annotation branch from 3cfa724 to b969b95 Compare January 28, 2024 17:22

Stephan202 commented Jan 28, 2024

View reviewed changes

Stephan202 force-pushed the sschroevers/introduce-refaster-type-migration-annotation branch from b969b95 to d71244f Compare February 10, 2024 13:29

rickie approved these changes Feb 11, 2024

View reviewed changes

rickie force-pushed the sschroevers/introduce-refaster-type-migration-annotation branch from eb7c402 to c7f03c7 Compare February 11, 2024 10:35

rickie mentioned this pull request Feb 11, 2024

Introduce NewStringFromCharArray{,SubSequence} Refaster rules #1012

Merged

Stephan202 and others added 9 commits February 11, 2024 12:20

Introduce ExhaustiveRefasterTypeMigration check

82f1519

The new `@TypeMigration` annotation can be placed on Refaster rule collections to indicate that they migrate most or all public methods of an indicated type. The new check validates the claim made by the annotation.

Fix JDK 11 compatibility

3bf11cb

Resolve some Sonarcloud warnings

fc70c3d

Post-rebase fixes

0ec05b7

Test overload support

a5049bd

Fix build

42dc15b

Move the check

81fb81d

Small typos and a annotation order swap

76db3dd

Tweak

3e54b8a

Stephan202 force-pushed the sschroevers/introduce-refaster-type-migration-annotation branch from c7f03c7 to 3e54b8a Compare February 11, 2024 11:30

Stephan202 commented Feb 11, 2024

View reviewed changes

Stephan202 merged commit 1cc792c into master Feb 11, 2024
16 checks passed

Stephan202 deleted the sschroevers/introduce-refaster-type-migration-annotation branch February 11, 2024 11:42

	String[] unmigratedMethods() default {};
	String[] nonMigratedMethods() default {};

	String[] unmigratedMethods() default {};
	String[] notMigratedMethods() default {};

	" \"publicStringMethodWithArg(int)\",",
	" \"publicStringMethodWithArg(int)\",",
	" \"publicStringMethodWithArg(String)\",",

	// XXX: Consider making this annotation `@Repeatable`, for cases where a single Refaster rules
	// XXX: Consider making this annotation `@Repeatable`, for cases where a single Refaster rule

Introduce ExhaustiveRefasterTypeMigration check #770

Introduce ExhaustiveRefasterTypeMigration check #770

Conversation

Stephan202 commented Aug 29, 2023 • edited Loading

github-actions bot commented Aug 29, 2023

github-actions bot commented Aug 29, 2023

sonarqubecloud bot commented Aug 29, 2023

github-actions bot commented Aug 29, 2023

github-actions bot commented Oct 8, 2023

sonarqubecloud bot commented Oct 8, 2023

sonarqubecloud bot commented Dec 18, 2023

Quality Gate passed

github-actions bot commented Dec 18, 2023

github-actions bot commented Dec 18, 2023

mohamedsamehsalah left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Stephan202 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jan 28, 2024

Stephan202 commented Feb 10, 2024

github-actions bot commented Feb 10, 2024

github-actions bot commented Feb 11, 2024

rickie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rickie Feb 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Feb 11, 2024

Stephan202 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Feb 11, 2024

sonarqubecloud bot commented Feb 11, 2024

Quality Gate passed

Introduce `ExhaustiveRefasterTypeMigration` check #770

Introduce `ExhaustiveRefasterTypeMigration` check #770

Stephan202 commented Aug 29, 2023 •

edited

Loading

rickie Feb 10, 2024 •

edited

Loading